-
-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multiple file upload button implementation #380 #434
base: main
Are you sure you want to change the base?
Multiple file upload button implementation #380 #434
Conversation
Hi thanks for showing interest in contributing. Overall your plan sounds great and well thought-out. Thanks for taking all the time to read through the code. Here's my feedback:
I think it is fine to use a union array as long as it's easy to tell which type each element is in code. But that would not be my first instinct since in any case you will need some
For this to make sense I think it would have to be completely obvious that the file is now available in the subtitle selector. Maybe even auto-select it in the first available drop-down. In any case I would hope that the multiple-files-in-different-tracks use-case could be solved by your solution efficiently, since that is the problem posed by the original issue. |
Thanks for the quick feedback :D I really appreciate your help so early in the PR
Yes, I agree, I was actually having trouble with type guarding with type script. I like the idea of using an optional field, (just some of the other fields would need to be optional like
Yes, good call out, auto-fill out sounds good to me too.
I think we get this for free, but I would also like the upload button to be used multiple times in case a user is unfamiliar with selecting multiple files, which emulates the functionality of having a separate upload button for each of the 3 tracks. 🙏 I will keep the original problem in mind and ensure the new flow does solve the original problem. |
Hi, a couple of thoughts from a fan of the project as well :)
I realise no. 2 adds quite a bit of complexity to this so it can be easily added as a follow up pr later if you deem it beneficial enough. Not wanting to add extra burden but just thinking of the ideal scenario :) |
There might be users who are only uploading 1 track but intending to use an embedded track for the secondary one. I guess this case makes sense if there are no embedded subtitles and they are uploading the files though, good call out 👍
This is actually a pain point for me right now. After the OK button is clicked, the language field in
I've been spiking this, and it actually gets quite messy, and we still eventually have to do if else statements. Displaying the options in the dropdown section assume url exists and is unique, so that logic also complicates things as well. I might reconsider the options I listed earlier (two arrays of tuple (obj + index), or heterogeneous array) |
Yep that's a good call. So maybe it's better to always display the popup, but we could also add a This once again complicates it, so probably worth doing in a separate pr as well :) |
Regarding auto-detected languages: My suggestion would be to not attempt to detect languages or modify language preference at all in the case of uploaded files. The language preference really only makes sense for auto-selecting one of the auto-detected subtitle tracks on the website. So I think user-provided files should not be used for this, only auto-detected subtitle tracks. |
Not sure if this is helpful, given the PR is already on course, but had another thought. We could mimic what MPV player does: it has a shortcut for selecting a subtitle for a given track. So, assuming that I've got .jp.srt and .en.srt files loaded, the shortcut would cycle between displaying Japanese, English or no subtitles for that track. So by using these shortcuts for both subtitle tracks, the user is able to effectively swap the diplayed subtitles positions |
Interesting, does MPV support multiple subtitle tracks at the same time? If there is interest in this feature, should probably make a new ticket, as it feels distinct from this work, beginner users like me won't be able to get used to shortcuts in-place of the UI. |
Yes, mpv has 2 subtitle tracks https://mpv.io/manual/master/#options-secondary-sid mpv.net overcomes this by providing UI to select a subtitle for the track, so we could do the same for ASB. But, like you say, job for another PR |
ConfirmedVideoDataSubtitleTrack is identical to VideoDataSubtitleTrack, except label is referred to as name, and url is referred to as subtitleUrl.
type guard will be needed later when I use the union type, to differentiate between the type of object.
6eb1e32
to
4eebc12
Compare
resolves #380
I'm a big fan of this project, so I decided to pick one of the tickets up to begin contributing to!
What
In order to complete this ticket, there needs to be some changes to how we currently process uploaded files.
Upload Path
The "Open Files" button runs a function which takes the uploaded file(s) from the input element and sends them to the BE as
SerializedSubtitleFile
.Complete Button Path
The "OK" button takes the selected subtitles, figures out which from
Confirmed
VideoDataSubtitleTrack[]
are selected, sends them to BE, and the Server will then serialise each one into aSerializedSubtitleFile
.In both cases, we eventually reach a
SerializedSubtitleFile
.The ask
The proposal I have to support #380 is to:
ConfirmedVideoDataSubtitleTrack
.handleFileInputChange
to not send theopenFile
message to bridge/server, but instead updatesubtitles[]
. That way, users can then select in the dropdown the newly uploaded subtitle files.SerializedSubtitleFile
) and video-parsed (VideoDataSubtitleTrack
) subtitles in theVideoDataUiBridgeConfirmMessage
. Current contenders are typescript union array, or two separate arrays of tuples which maintain order.SerializedSubtitleFile
as-is, but doing the same processing as before forVideoDataSubtitleTrack
openFile
message/command to server, as now uploads are handled byconfirm
.The user flow will look like this instead:
Notes:
ConfirmedVideoDataSubtitleTrack
, as it seems to be a duplicated class providing little utility, so I've merged it into one interface.